You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(workspace-fs): wait for released watchers during shutdown - #9662
A caller can stop the last listener, await filesystem shutdown, and then remove its checkout while the native watcher is still attached. The release path removes the entry from the path map immediately (so another listener can reattach), but that also hid the in-flight teardown from FsWatcherManager.close().
Keep that cleanup visible to shutdown, both when registration is in flight and when the watch is already live. Reattachment still gets a fresh entry; the existing timeout for a dead native stream is unchanged.
Found while verifying #9625 on macOS. The original crash report shows FSEventsCallback → FSEventStreamStop in Parcel's deleted-root path. The early return from shutdown is reproducible; the native SIGTRAP itself did not recur on the unchanged PR in the latest default-worker run. This fixes a teardown race, not every possible native watcher crash.
How I tested it
Held native cleanup behind a gate and checked whether shutdown observed a detached watcher. Both the attaching and attached cases failed before the fix and passed after it.
On the fix(desktop): let IME users confirm text without submitting a chat answer #9625 checkout with this patch, macOS arm64, Bun 1.4.3: host-service 2,861 passed, workspace-fs 196 passed. Used the normal package scripts, default worker count and default timeout. On the same checkout before the patch, an unrelated Git directory worker test timed out once; no SIGTRAP that run.
Fixes a shutdown race in FsWatcherManager where close() could resolve while the last listener's native watch was still tearing down, crashing when the checkout was removed immediately after.
Tracks in-flight watcher releases so close() waits on them alongside still-registered watchers.
Reattachment still gets a fresh watcher entry; the dead-stream timeout is unchanged.
Written for commit 64487db. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Workspace shutdown now waits for in-progress file-watcher setup and cleanup to finish before completing. This improves reliability when watchers are starting or stopping as the workspace closes, and helps prevent shutdown from completing while watcher cleanup is still pending.
No actionable comments were generated in the recent review.
Walkthrough
FsWatcherManager tracks in-progress final-listener releases. Its close() method waits for those releases and registered watcher disposal. A new test checks shutdown while native watcher attachment and detachment are gated.
Changes: Watcher shutdown
Layer / File(s)
Summary
Track releases during shutdown packages/workspace-fs/src/watch.ts, packages/workspace-fs/src/watch-cancellation.test.ts
Final-listener release tracks pending watcher creation and disposal. close() waits for tracked releases as well as registered watcher disposal. The test covers both attachment phases and verifies that close() waits for unsubscribe completion.
ParadevAI/Paradev#9460 describes a native watcher subscription resolving after disposal. This change tracks in-progress releases and makes close() wait for them; full resolution of the issue is unconfirmed.
Possibly related PRs
ParadevAI/Paradev#7878: Both changes define watcher lifecycle behavior for final-listener release and close().
Suggested reviewers:kitenite
Merge Risk: Minimal · up to 64487
This change makes watcher shutdown wait for in-progress teardown. No actionable merge-blocking risk is evident from the supplied review.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title "fix(workspace-fs): wait for released watchers during shutdown" matches the main change. The watch.ts change makes FsWatcherManager.close() wait for tracked release work (watcher creation or…
Description check
The description covers the problem and fix in 'What & why', with clear context on the teardown race and why the path map cleanup hid in-flight teardown from close(). The 'How I tested it' section give…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title "fix(workspace-fs): wait for released watchers during shutdown" matches the main change. The watch.ts change makes FsWatcherManager.close() wait for tracked release work (watcher creation or disposal) after the final listener is removed from the path map. It follows conventional commit format and is specific to the package and behavior changed.
Full details: Description check
Explanation
The description covers the problem and fix in 'What & why', with clear context on the teardown race and why the path map cleanup hid in-flight teardown from close(). The 'How I tested it' section gives concrete commands, platform, test counts, and the gated test results for both attaching and attached cases. The checklist is present and mostly checked. The template asks for conventional commit titles, lint/typecheck, maintainer edits, and CLA, and the author marks each item. The description does not include screenshots, which the template prefers only for user-visible changes; this change is internal, so that omission is acceptable. It also links the related issue (#9625) and a related issue (#9658), though 'fixes #...' is not used.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
A caller can stop the last listener, await filesystem shutdown, and then remove its checkout while the native watcher is still attached. The release path removes the entry from the path map immediately (so another listener can reattach), but that also hid the in-flight teardown from
FsWatcherManager.close().Keep that cleanup visible to shutdown, both when registration is in flight and when the watch is already live. Reattachment still gets a fresh entry; the existing timeout for a dead native stream is unchanged.
Found while verifying #9625 on macOS. The original crash report shows
FSEventsCallback → FSEventStreamStopin Parcel's deleted-root path. The early return from shutdown is reproducible; the native SIGTRAP itself did not recur on the unchanged PR in the latest default-worker run. This fixes a teardown race, not every possible native watcher crash.How I tested it
Checklist
Summary by cubic
Fixes a shutdown race in
FsWatcherManagerwhereclose()could resolve while the last listener's native watch was still tearing down, crashing when the checkout was removed immediately after.close()waits on them alongside still-registered watchers.Written for commit 64487db. Summary will update on new commits.
Summary by CodeRabbit